Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bounded stack #156

Merged
merged 11 commits into from
Nov 21, 2024
Merged

Bounded stack #156

merged 11 commits into from
Nov 21, 2024

Conversation

lyrm
Copy link
Collaborator

@lyrm lyrm commented Oct 28, 2024

Based on #155. This PR removes the blocking part of the previous PR. The issue with it is that it adds a dependency to picos, probably requiring to create a Saturn_picos package.

For Saturn.1.0, we have decided to do only a bounded stack, thus this PR.

@lyrm lyrm mentioned this pull request Oct 28, 2024
Closed
4 tasks
@lyrm lyrm force-pushed the bounded_stack_non_blocking branch 5 times, most recently from cbf8675 to 2970bf6 Compare October 31, 2024 22:01
@lyrm lyrm mentioned this pull request Oct 31, 2024
Merged
@lyrm
Copy link
Collaborator Author

lyrm commented Nov 1, 2024

@Sudha247 : Could you have a look at this PR (at the documentation, or even a quick look at the tests, maybe ?), any feedbacks will be great !

@lyrm lyrm force-pushed the bounded_stack_non_blocking branch 2 times, most recently from 1a9fc60 to 5677178 Compare November 5, 2024 16:44
art-w
art-w reviewed Nov 7, 2024
View reviewed changes
src/bounded_stack.ml Outdated Show resolved Hide resolved
@lyrm lyrm mentioned this pull request Nov 15, 2024
Merged
7 tasks
art-w
art-w approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@art-w art-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Some minor comments

src/bounded_stack.ml Outdated Show resolved Hide resolved
src/bounded_stack.ml Outdated Show resolved Hide resolved
@lyrm lyrm force-pushed the bounded_stack_non_blocking branch from 2f4b02f to 8ad3ea6 Compare November 21, 2024 15:44
@lyrm
Copy link
Collaborator Author

lyrm commented Nov 21, 2024

@art-w I think this PR is good to. What do you think ?

art-w
art-w approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@art-w art-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it looks good! Out of curiosity, are the copy_as_padded on records necessary? (my understanding is that this makes the allocated object fatter, so as to avoid false-sharing contention, but the head atomic already appears to be protected by make_contended)

@lyrm
Copy link
Collaborator Author

lyrm commented Nov 21, 2024

Mmh there is a stack overflow on Treiber_stack.push_all. This PR has almost the same implementation so I will have a look before merging !

@lyrm
Copy link
Collaborator Author

lyrm commented Nov 21, 2024

Yes it looks good! Out of curiosity, are the copy_as_padded on records necessary? (my understanding is that this makes the allocated object fatter, so as to avoid false-sharing contention, but the head atomic already appears to be protected by make_contended)

Good question. As nothing else is mutable in the stack type, probably not.

@lyrm lyrm force-pushed the bounded_stack_non_blocking branch from c3cc626 to 8471c97 Compare November 21, 2024 17:59
@lyrm lyrm merged commit e50b636 into ocaml-multicore:main Nov 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@art-w art-w art-w approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lyrm @art-w